PR #284 Review: Incremental Graph Rebuild (G1+G2+G3)

+2003 / -67 lines across 8 files — architecture is sound, but there's a critical runtime bug that must be fixed before merge.

Critical Bug: _write_clients_producers_and_calls will crash at runtime

This function has multiple issues that would cause failures on any real codebase with routes/clients/producers:

1. Wrong parameter names — The Cypher templates use $sid/$cid/$pid/$rid for MATCH node lookups, but the function passes $src/$dst:

# _CREATE_DECLARES_CLIENT expects: MATCH (s:Symbol {id: $sid}), (c:Client {id: $cid})
conn.execute(_CREATE_DECLARES_CLIENT, {
    "src": row.symbol_id,   # BUG: should be "sid"
    "dst": row.client_id,   # BUG: should be "cid"
    ...
})

Same mismatch for DECLARES_PRODUCER ($sid/$pid → "src"/"dst"), HTTP_CALLS ($cid/$rid → "src"/"dst"), and ASYNC_CALLS ($pid/$rid → "src"/"dst").

2. Missing parameters — All four edge-writing loops omit required template parameters:

3. O(n²) lookup — Source file resolution for HTTP_CALLS/ASYNC_CALLS builds a new list and does .index() for every row:

tables.client_rows[[c.id for c in tables.client_rows].index(row.client_id)].filename

Should use a dict lookup like the existing _write_routes_and_exposes does.

Why tests don't catch this: The minimal test fixtures (class A {}, class B extends A {}) have no routes/clients/producers, so these loops are never entered. A test with Spring-annotated classes would surface the crash.

Significant Issues (should fix)

4. Duplicated _load_existing_types / _load_existing_types_filtered — These two functions (and their _members counterparts) are ~50 lines each and differ only by the AND NOT (s.filename IN $exclude_files) clause. Extract common logic into a single function with an optional exclude_files param.

5. Repetitive _find_dependents — Six nearly identical if/elif branches that only differ by the edge label name. Loop over a list of edge type strings instead.

6. _write_nodes_merge duplicates _write_nodes — These ~70-line functions differ only by using _MERGE_SYMBOL vs _CREATE_SYMBOL. Factor to a shared helper that accepts the query template as a parameter.

7. _file_by_node_id built twice independently — Once in _write_edges and once in _write_routes_and_exposes. Compute once and share.

Minor Issues (nice to fix)

8. Duplicate comment in pass1_parse — "Skip files not in scope" appears twice.

9. import json inside main() and _cmd_increment — should be top-level imports.

10. FileHashTracker.save() silently swallows OSError — consider logging a warning so it's discoverable when writes fail repeatedly.

11. _fallback_to_full duplicates hash-init logic from the no-DB branch of incremental_rebuild — extract to a shared helper.

12. AGENTS.md says ontology_version is 15 but is now 17 — worth updating.

Test Coverage Gap

No test with Spring annotations (routes, clients, producers) that exercises the pass 5-6 global path with actual Client/Producer/HTTP_CALLS/ASYNC_CALLS edges. This is why the critical bug above slipped through. Adding one test against the bank-chat-system fixture (which has Spring controllers) would catch it.

Summary

The design — phase-based deletion, single-hop dependent expansion with cap, crash marker, automatic fallback — is well-considered. The critical bug in _write_clients_producers_and_calls (wrong param names + missing params) must be fixed before merge. The duplication issues are worth addressing but not blocking.